-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update UI dependencies #33
Update UI dependencies #33
Conversation
This PR is larger than the one for Turing. 😱 😅 @terryyylim could you have the first pass at this when you get to it? I'll be the second set of eyes this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Largely LGTM, just some small comments to address. Thank you for the PR, @deadlycoconuts!
</EuiCallOut> | ||
) : ( | ||
<Fragment> | ||
{!(props["*"] === "edit") ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do {!(props["*"] === "edit") && (...)
here instead so we can be more concise and remove <></>
?
</EuiPanel> | ||
</ConfigSection> | ||
<Fragment> | ||
<EuiSpacer size="m" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this EuiSpacer
gets removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
const ExperimentHistoryDetailsView = ({ projectId, experimentId, version }) => { | ||
const { appConfig } = useConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we standardise with other files, to just extract variables we need? i.e restrictWidth
and paddingSize
in this case.
@@ -78,14 +76,14 @@ const SearchExperimentFilters = ({ onChange }) => { | |||
|
|||
return ( | |||
<> | |||
<EuiFlyoutHeader hasBorder className="searchPanelFlyoutHeader"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just works? 🎉 Just curious, was this somehow fixed in some version of EUI which was after our current version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it just does. I don't know why the original header's margin was stretched by an additional 20px to the right; maybe it's to enable the 'Reset' button to lie further right than could be done normally?
The same effect is now achieved by making the EuiFlexItem
containing the button not grow, which causes the EuiFlexGroup
to allocate more space for the 'Filters' header instead:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've partially reviewed this PR and left a few small comments. Will complete it on Monday.
@@ -34,7 +34,7 @@ export const BasicTable = ({ | |||
iconType="alert"> | |||
<p>{error.message}</p> | |||
</EuiCallOut> | |||
) : !!onPaginationChange && !!totalItemCount & !!page ? ( | |||
) : !!onPaginationChange && !!totalItemCount && !!page ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. 😅
ui/src/settings/components/playground_flyout/PlaygroundFlyout.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the changes @deadlycoconuts 👍🏻 We can merge it once @krithika369 has another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too. Thanks for this PR and refactoring some of the components along the way. I left some small comments. Please feel free to merge once they have been addressed.
@@ -2,11 +2,11 @@ export const getSegmenterScope = (scope) => { | |||
const status = { | |||
project: { | |||
label: "Project", | |||
color: "primary", | |||
color: "#07C", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do primary / success no longer work for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I made a mistake with the changes here; I thought the primary colours weren't working with primary
/success
specified because they seemed to be of a paler shade (so I changed them manually to the brighter ones we see in other parts of the new EUI theme):
But after checking out the docs again for EuiBadge
, it seems like they are indeed of the intended shade:
I'll change these values back to primary
/success
in this case then 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the default color palette is fine. Not sure why the Eui team decided to keep the old colors for some components like Text and this one..
@@ -19,19 +19,19 @@ export const getExperimentStatus = (experiment) => { | |||
}, | |||
Completed: { | |||
label: "Completed", | |||
color: "#017D73", | |||
color: "#00BFB3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace the color
values here with the healthColor
values (and replace the property name healthColor
with color
in the app)? The current color
values are only used in the Eui badge from what I see and they should be able to accept primary / success / ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue here as the above - it seems like EuiHealth
also uses a darker shade (kind of like in the older versions of the theme) of all the colours instead of the more exciting ones we've been seeing. I should checked this more thoroughly before I guess 👀 In any case, I've replaced the color
values with the healthColor
values (subdued
/success
/warning
/primary
) and removed the healthColor
values completely. Any component that used to consume healthColor
values now consume the new (reverted) color
values.
initialIsOpen={variables.length > 1} | ||
buttonContent={buttonContent} | ||
arrowDisplay="none" | ||
extraAction={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for this.
ui/src/settings/components/playground_flyout/PlaygroundFlyout.scss
Outdated
Show resolved
Hide resolved
…ents and segments
6575502
to
91d4c9b
Compare
Thank you @terryyylim and @krithika369 for the detailed review! I should've addressed all (if not most 👀) of all the comments that both of you have left 🙂 I'll be merging this soon once the CI pipeline passes 🚀 |
Context
With the update of
mlp-ui
dependencies in caraml-dev/mlp#54 and caraml-dev/mlp#55 (in particular the update ofNode.js
14 → 16,React
16 → 17,@elastic/eui
32.3.0 → 66.0.0 andreact-scripts
to 5.0.1), there is a need to similarly update XP UI, as it uses components from and is dependent on themlp-ui
package.Main Modifications
Empty
component with the newPage404
componentEuiPageTemplate
(see https://elastic.github.io/eui/#/templates/page-template) to manage page layouts; this replaces the existing usage ofEuiPage
,EuiPageHeader
,EuiPageSection
, etc. componentsv66
of@elastic/eui
(see https://eui.elastic.co/#/theming/colors/values)EuiFlyout
sFixes
Inconsistent spacing between page title and MLP bar
Before
Experiment/Treatment/Segment Details View: Experiment/Treatment/Segment Edit View:
.
After
Experiment/Treatment/Segment Details View: Experiment/Treatment/Segment Edit View:
.
Known Issues (Unable to be addressed in this PR)
mlp-ui
to fix theStepsWizardHorizontal
component as it does not seem to be highlighting as users progress through the timeline steps: